Write output to stdout - #108
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements the ability to write the current JSON result to stdout when exiting the application (fixing issue #40), alongside a major refactoring of the configuration system to use the termcfg library and directly embed promkit-widgets configuration types.
Changes:
- Added
--write-to-stdoutflag that captures the filtered JSON output and writes it to stdout on exit (Unix-only when stdout is piped) - Migrated configuration system from custom event matching and content style serialization to the
termcfglibrary, simplifying the codebase significantly - Updated JSON viewer error handling to always show the original JSON alongside error messages, improving user experience
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.rs | Added --write-to-stdout flag, implemented StdoutRedirect for Unix to handle /dev/tty redirection, and integrated output writing on exit |
| src/prompt.rs | Changed return type to Option<String> to support stdout output, added mouse wheel event normalization for scroll bindings, migrated keybind checks from custom matches() to HashSet::contains() |
| src/config.rs | Major refactoring to use termcfg for event/style serialization and directly embed promkit-widgets config types (removed 200+ lines of custom code) |
| src/config/event.rs | Removed entire file (104 lines) - custom event matching system replaced by HashSet<Event> with termcfg |
| src/config/content_style.rs | Removed entire file (65 lines) - custom style serialization replaced by termcfg notation |
| src/config/text_editor.rs | Removed entire file (33 lines) - custom Mode serialization replaced by library types |
| src/json.rs | Updated to use new config types, improved error handling to always restore original JSON stream on errors |
| src/editor.rs | Simplified focus/defocus to swap entire config object instead of individual fields |
| src/search.rs | Updated API calls for new promkit-widgets version (from_displayable → from, state.lines → state.config.lines) |
| default.toml | Complete syntax overhaul to use termcfg notation (e.g., "fg=blue" instead of verbose object syntax) and added mouse scroll bindings |
| README.md | Updated documentation for new configuration syntax with detailed comments about termcfg notation and migration notes |
| Cargo.toml | Upgraded promkit-widgets to 0.3.0, added termcfg and libc dependencies |
| Cargo.lock | Updated dependencies including promkit-widgets, termcfg, and various transitive dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .read(true) | ||
| .write(true) | ||
| .open("/dev/tty") | ||
| .map_err(|e| anyhow!("Failed to open /dev/tty for TUI rendering: {e}"))?; |
There was a problem hiding this comment.
The error message when /dev/tty cannot be opened could be more helpful for users. Consider adding a suggestion to the error message, such as: "Failed to open /dev/tty for TUI rendering. This typically happens in headless environments or containers. Try running without --write-to-stdout, or ensure /dev/tty is accessible." This would help users understand why the feature doesn't work in certain environments (e.g., CI/CD pipelines, Docker containers without TTY allocation).
| .map_err(|e| anyhow!("Failed to open /dev/tty for TUI rendering: {e}"))?; | |
| .map_err(|e| anyhow!( | |
| "Failed to open /dev/tty for TUI rendering. This typically happens in headless environments or containers. \ | |
| Try running without --write-to-stdout, or ensure /dev/tty is accessible: {e}" | |
| ))?; |
| .map_err(|e| anyhow!("Failed to read configuration file: {e}")) | ||
| }) | ||
| .and_then(|content| Config::load_from(&content)) | ||
| .unwrap_or_else(|_e| { |
There was a problem hiding this comment.
When config file loading fails, the error is silently discarded and the application falls back to the default configuration without notifying the user. Users with syntax errors or invalid configurations in their config.toml won't know why their settings aren't being applied. Consider logging a warning message (e.g., using eprintln!) before falling back to the default configuration, so users are aware their custom config couldn't be loaded and can fix any issues.
| .unwrap_or_else(|_e| { | |
| .unwrap_or_else(|e| { | |
| eprintln!( | |
| "Warning: Failed to load configuration from file, falling back to default: {e}" | |
| ); |
70bcfad to
0908e1a
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.